Skip to content

Add WebSocketStream #116729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

stephentoub
Copy link
Member

Fixes #111217

Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new WebSocketStream abstraction to forward Stream operations over a WebSocket and includes accompanying tests, resource updates, and API surface adjustments.

  • Introduces WebSocketStream and related specialized stream types (ReadWriteStream, WriteMessageStream, ReadMessageStream)
  • Adds comprehensive tests in WebSocketStreamTests.cs and integrates them into the test project
  • Extends ManagedWebSocket to centralize message-type validation and updates resources and reference assemblies

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs Added WebSocketStream implementation
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs Extracted ThrowIfInvalidMessageType helper
src/libraries/System.Net.WebSockets/src/Resources/Strings.resx Added net_WebSockets_TimeoutOutOfRange string
src/libraries/System.Net.WebSockets/ref/System.Net.WebSockets.cs Updated reference assembly for WebSocketStream
src/libraries/System.Net.WebSockets/tests/System.Net.WebSockets.Tests.csproj Updated test project to include new tests
src/libraries/System.Net.WebSockets/tests/WebSocketStreamTests.cs Added tests for new WebSocketStream behavior
src/libraries/Common/tests/StreamConformanceTests/System/IO/StreamConformanceTests.cs Updated conformance tests to async patterns
Comments suppressed due to low confidence (4)

src/libraries/System.Net.WebSockets/src/Resources/Strings.resx:142

  • [nitpick] The error message is unclear; consider rephrasing to "The timeout must be non-negative or Timeout.InfiniteTimeSpan." for better readability.
    <value>The timeout must be a value between non-negative or Timeout.InfiniteTimeSpan.</value>

src/libraries/System.Net.WebSockets/tests/System.Net.WebSockets.Tests.csproj:11

  • The project references WebSocketCreateTest.cs which does not exist; it should reference the actual test file (WebSocketStreamCreateTests.cs) to include the new tests and avoid compilation failures.
    <Compile Include="WebSocketCreateTest.cs" />

src/libraries/System.Net.WebSockets/ref/System.Net.WebSockets.cs:51

  • The reference assembly declares a parameterless WebSocketStream constructor, but the implementation only defines a constructor taking a WebSocket parameter. This mismatch will cause API and compilation inconsistencies.
        private protected WebSocketStream() { }

src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs:273

  • The call to ConfigureAwait uses ConfigureAwaitOptions.SuppressThrowing, but ConfigureAwait expects a bool. Replace with ConfigureAwait(false) (or true if appropriate) to match the API.
                                await WebSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, null, ct).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);

Comment on lines +372 to +376
public override ValueTask DisposeAsync()
{
_disposed = true;
return default;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood Natalia correctly, we might want to abort the WebSocket here if we haven't hit EOF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add APIs to WebSocket which allow it to be read as a Stream
3 participants